Use per-tx escrow for Wei balance #1314
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -167,6 +167,7 @@ | |||
| // returns no validator updates. | |||
| func (am AppModule) EndBlock(ctx sdk.Context, _ abci.RequestEndBlock) []abci.ValidatorUpdate { | |||
| evmTxDeferredInfoList := am.keeper.GetEVMTxDeferredInfo(ctx) | |||
| am.keeper.SettleWeiEscrowAccounts(ctx, evmTxDeferredInfoList) | |||
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| if err := k.BankKeeper().SendCoinsFromAccountToModule(ctx, escrow, banktypes.WeiEscrowName, sdk.NewCoins(seiBalance)); err != nil { | ||
| ctx.Logger().Error(fmt.Sprintf("failed to send %s from escrow %d to global escrow", seiBalance.String(), info.TxIndx)) | ||
| // This should not happen in any case. We want to halt the chain if it does | ||
| panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| if err := k.BankKeeper().SendCoinsFromModuleToAccount(ctx, banktypes.WeiEscrowName, escrow, sdk.NewCoins(settleAmt)); err != nil { | ||
| ctx.Logger().Error(fmt.Sprintf("failed to send %s from global escrow to escrow %d", settleAmt.String(), info.TxIndx)) | ||
| // This should not happen in any case. We want to halt the chain if it does | ||
| panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| escrow := state.GetTempWeiEscrowAddress(sdk.Context{}.WithTxIndex(info.TxIndx)) | ||
| seiBalance := k.BankKeeper().GetBalance(ctx, escrow, denom) | ||
| if !seiBalance.Amount.IsZero() { | ||
| panic(fmt.Sprintf("failed to settle escrow account %d which still has a balance of %s", info.TxIndx, seiBalance.String())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| @@ -167,6 +167,7 @@ func (am AppModule) BeginBlock(sdk.Context, abci.RequestBeginBlock) { | |||
| // returns no validator updates. | |||
| func (am AppModule) EndBlock(ctx sdk.Context, _ abci.RequestEndBlock) []abci.ValidatorUpdate { | |||
| evmTxDeferredInfoList := am.keeper.GetEVMTxDeferredInfo(ctx) | |||
| am.keeper.SettleWeiEscrowAccounts(ctx, evmTxDeferredInfoList) | |||
There was a problem hiding this comment.
in the future, if we allow evm txs into the prioritized execution section, is it necessary to settle the balances after EACH execution group, or is it fine to simply settle in endblock?
There was a problem hiding this comment.
it's fine to settle in the endblock. Technically I don't think we ever need to settle, but having negative balances may break some UI (if someone deliberately query the escrow accounts) so we settle it, but as long as end-of-block state is settled we should be ok
| func GetTempWeiEscrowAddress(ctx sdk.Context) sdk.AccAddress { | ||
| txIndexBz := make([]byte, 8) | ||
| binary.BigEndian.PutUint64(txIndexBz, uint64(ctx.TxIndex())) | ||
| return sdk.AccAddress(append(WeiTmpEscrowPrefix, txIndexBz...)) |
There was a problem hiding this comment.
do we have a guarantee that this cannot conflict with other user addresses? I'm assuming it wont because there are fewer bytes used in generating the Acc Address?
There was a problem hiding this comment.
yeah the bytes are different
x/evm/state/utils.go
Outdated
| @@ -26,6 +27,12 @@ func GetCoinbaseAddress(ctx sdk.Context) sdk.AccAddress { | |||
| return sdk.AccAddress(append(CoinbaseAddressPrefix, txIndexBz...)) | |||
| } | |||
|
|
|||
| func GetTempWeiEscrowAddress(ctx sdk.Context) sdk.AccAddress { | |||
There was a problem hiding this comment.
can't we just use txIndex as an argument instead of Context?
x/evm/keeper/wei.go
Outdated
| denom := k.GetBaseDenom(ctx) | ||
| // settle surplus escrow first | ||
| for _, info := range evmTxDeferredInfoList { | ||
| escrow := state.GetTempWeiEscrowAddress(sdk.Context{}.WithTxIndex(info.TxIndx)) |
There was a problem hiding this comment.
see comment on utils file, might be easier to simply pass txIndex instead of creating an empty context WITH a txindex?
codchen
force-pushed
the
evm-wei-balance-para
branch
2 times, most recently
from
8cb3ab4 to
1d9f623
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## evm #1314 +/- ##
==========================================
- Coverage 63.30% 63.15% -0.16%
==========================================
Files 349 350 +1
Lines 23522 23560 +38
==========================================
- Hits 14891 14879 -12
- Misses 7787 7833 +46
- Partials 844 848 +4
|
codchen
force-pushed
the
evm-wei-balance-para
branch
from
1d9f623 to
1953b9f
Compare
* Use per-tx escrow for Wei balance * rebase
* Use per-tx escrow for Wei balance * rebase
* Use per-tx escrow for Wei balance * rebase
* Use per-tx escrow for Wei balance * rebase
* Use per-tx escrow for Wei balance * rebase
* Use per-tx escrow for Wei balance * rebase
* Use per-tx escrow for Wei balance * rebase
* Use per-tx escrow for Wei balance * rebase
* Use per-tx escrow for Wei balance * rebase
* Use per-tx escrow for Wei balance * rebase
* Use per-tx escrow for Wei balance * rebase
* Use per-tx escrow for Wei balance * rebase
* Use per-tx escrow for Wei balance * rebase
Describe your changes and provide context
In order to prevent the Wei escrow from being a bottleneck in OCC, this PR changes the escrow account to one-per-tx, and settle those per-tx escrows to the global escrow in EndBlock. One complication with such setup is that the per-tx escrow may temporarily go negative (because the
useithat needs to be redeemed may be deposited in another tx), so a corresponding change in sei-cosmos that skips negative balance check for escrow account has also been made sei-protocol/sei-cosmos#417Testing performed to validate your change
unit tests with different escrow balances